Codecov Report

Merging #39 (79cd57b) into master (bd3d50d) will decrease coverage by 1.63%.
The diff coverage is 68.35%.

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   87.06%   85.43%   -1.64%     
==========================================
  Files          28       29       +1     
  Lines         866      927      +61     
==========================================
+ Hits          754      792      +38     
- Misses        112      135      +23     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd3d50d...54bb69a. Read the comment docs.

I don't like how we allow both a str and passing arguments via kwargs. It is ugly and hard for users to know what parameters are available

I think it would be cleaner to allow str to create the callback with some sensible defaults

And if the user wants to set their own parameters, have them instantiate the callback themselves and pass it.

# unfreezes at epoch 1 by default
trainer.finetune(model, datamodule=datamodule, strategy='freeze_unfreeze')

# if you want to change it
trainer.finetune(model, datamodule=datamodule, strategy=FreezeUnfreeze(unfreeze_epoch=50))

I don't like how we allow both a str and passing arguments via kwargs. It is ugly and hard for users to know what parameters are available

I think it would be cleaner to allow str to create the callback with some sensible defaults

And if the user wants to set their own parameters, have them instantiate the callback themselves and pass it.

# unfreezes at epoch 1 by default
trainer.finetune(model, datamodule=datamodule, strategy='freeze_unfreeze')

# if you want to change it
trainer.finetune(model, datamodule=datamodule, strategy=FreezeUnfreeze(unfreeze_epoch=50))

Sounds fine to me !